Skip to content

Fix bug: cookie.expires not modified even if maxAge set - #277

Closed
jjqq2013 wants to merge 5 commits into
expressjs:masterfrom
sjitech:master
Closed

Fix bug: cookie.expires not modified even if maxAge set#277
jjqq2013 wants to merge 5 commits into
expressjs:masterfrom
sjitech:master

Conversation

@jjqq2013

@jjqq2013 jjqq2013 commented Feb 20, 2016

Copy link
Copy Markdown

ref: #276.

Hi, i found when access server at second time, cookie.expire will not be modified even if i set maxAge and rolling:true.

Here is my test code.

var express = require('express');
var path = require('path');
var cookieParser = require('cookie-parser');
var bodyParser = require('body-parser');
var session = require('express-session');

var app = express();

// Use the session middleware
app.use(session({
  secret: 'keyboard cat',
  rolling: true,
  saveUninitialized: true,
  resave: true,
  cookie: { maxAge: 10000 }}))

// Access the session as req.session
app.get('/', function(req, res, next) {
  var sess = req.session
  if (sess.views) {
    sess.views++
    res.setHeader('Content-Type', 'text/html')
    res.write('<p>views: ' + sess.views + '</p>')
    res.write('<p>expires in: ' + (sess.cookie.maxAge / 1000) + 's</p>')
    res.end()
  } else {
    sess.views = 1
    res.end('welcome to the session demo. refresh!')
  }
});

In above example, i set maxAge to 10 seconds, and rolling:true, and saveUninitialized:true. Other settings seems have no effect to this problem.

I run this code, then use following curl command line to test and see verbose output:

$ curl --verbose --cookie cookie --cookie-jar cookie http://localhost:3000
* Rebuilt URL to: http://localhost:3000/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.43.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< X-Powered-By: Express
* Replaced cookie connect.sid="s%3AGRecnYYVv0WP0plY8qrIvV4EHG82Ts8h.aGG7rL0Fdb4fiXS0zN3u5eC9h%2BY8adZgZxzpyFZWFNs" for domain localhost, path /, expire 1455991539
< set-cookie: connect.sid=s%3AGRecnYYVv0WP0plY8qrIvV4EHG82Ts8h.aGG7rL0Fdb4fiXS0zN3u5eC9h%2BY8adZgZxzpyFZWFNs; Path=/; Expires=Sat, 20 Feb 2016 18:05:39 GMT; HttpOnly
< Date: Sat, 20 Feb 2016 18:05:29 GMT
< Connection: keep-alive
< Transfer-Encoding: chunked
< 
* Connection #0 to host localhost left intact
welcome to the session demo. refresh!

After about 5 seconds, i run the command line again, it will use same cookie from last response.

$ curl --verbose --cookie cookie --cookie-jar cookie http://localhost:3000
* Rebuilt URL to: http://localhost:3000/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.43.0
> Accept: */*
> Cookie: connect.sid=s%3AGRecnYYVv0WP0plY8qrIvV4EHG82Ts8h.aGG7rL0Fdb4fiXS0zN3u5eC9h%2BY8adZgZxzpyFZWFNs
> 
< HTTP/1.1 200 OK
< X-Powered-By: Express
< Content-Type: text/html
* Replaced cookie connect.sid="s%3AGRecnYYVv0WP0plY8qrIvV4EHG82Ts8h.aGG7rL0Fdb4fiXS0zN3u5eC9h%2BY8adZgZxzpyFZWFNs" for domain localhost, path /, expire 1455991539
< set-cookie: connect.sid=s%3AGRecnYYVv0WP0plY8qrIvV4EHG82Ts8h.aGG7rL0Fdb4fiXS0zN3u5eC9h%2BY8adZgZxzpyFZWFNs; Path=/; Expires=Sat, 20 Feb 2016 18:05:39 GMT; HttpOnly
< Date: Sat, 20 Feb 2016 18:05:35 GMT
< Connection: keep-alive
< Transfer-Encoding: chunked
< 
* Connection #0 to host localhost left intact
<p>views: 2</p><p>expires in: 4.065s</p>

Please notice the expire of second response, i'v set maxAge to 10 seconds, but cookie's expire will be:

        Expires=Sat, 20 Feb 2016 18:05:39 GMT
Current time is: Sat, 20 Feb 2016 18:05:35 GMT

It means expire after 4 seconds, this is not by-design obviously.

I checked source code in index.js, line 206:

    onHeaders(res, function(){
      ... ...
      var cookie = req.session.cookie;
      ... ...
//line 206:
      setcookie(res, name, req.sessionID, secrets[0], cookie.data);

*I insert following code to line 206 to change cookie.expire* then works!

      cookie.originalMaxAge > 0 && (cookie.expires = new Date(Date.now() + cookie.originalMaxAge));

I hope you can check this and merge it.

@dougwilson

dougwilson commented Feb 20, 2016

Copy link
Copy Markdown
Contributor

Hi! Please update your change to use an if statement. Also, please add a test to our test suite that fails without your fix and passes with your fix.

@jjqq2013

jjqq2013 commented Feb 21, 2016

Copy link
Copy Markdown
Author

Hi i have added test case for this problem.
Finally i found the problem occurs only when using res.write(...), res.end() combo.

res.write(...)
res.end()

My test case:

  describe('cookie.expire', function(){
    this.timeout(86400000);
    var val;

    var app = express()
        .use(session({ secret: 'keyboard cat', cookie: { maxAge: 10000 }}))  //timeout:10 seconds
        .use(function(req, res, next){
          req.session.count = req.session.count || 0;
          req.session.count++;
          res.write(req.session.count.toString());  //this is very important. If use res.end(...) then no bug
          res.end('');
        });

    it('should be Now + .maxAge  (first test)', function(done){
      request(app)
          .get('/')
          .expect(200, '1', function (err, res) {
            var a = new Date(expires(res))
            var b = new Date
            var delta = a.valueOf() - b.valueOf()

            val = cookie(res).split(';')[0];

            assert.ok(delta > 9000 && delta <= 10000, "cookie.expire is too short as expected");
            done();
          });
    });

    it('should be Now + .maxAge (second test)', function(done){
      setTimeout(function() { //do this after 6 seconds, still not timeout
        request(app)
            .get('/')
            .set('Cookie', val)
            .expect(200, '2', function (err, res) {
              var a = new Date(expires(res))
              var b = new Date
              var delta = a.valueOf() - b.valueOf()

              var thisCookie = cookie(res).split(';')[0];
              assert.equal(thisCookie, val, "cookie value itself should be same");

              assert.ok(delta > 9000 && delta <= 10000, "cookie.expire is too short as expected");
              done();
            });
      }, 6000);
    });
  });

My test result:
Before modification:

$ npm test
... ...
  session()
    cookie.expire
      ✓ should be Now + .maxAge  (first test) (45ms)
      1) should be Now + .maxAge (second test)

  1 passing (6s)
  1 failing

  1) session() cookie.expire should be Now + .maxAge (second test):

      Uncaught AssertionError: cookie.expire is too short as expected
      + expected - actual

      -false
      +true

      at Test.<anonymous> (test/session.js:59:22)
      at Test.assert (node_modules/supertest/lib/test.js:156:6)
      at Server.assert (node_modules/supertest/lib/test.js:127:12)
      at emitCloseNT (net.js:1514:8)
... ...

After modification:

$ npm test
... ...
  session()
    cookie.expire
      ✓ should be Now + .maxAge  (first test) (58ms)
      ✓ should be Now + .maxAge (second test) (6017ms)
  2 passing (6s)

I don't dig too deep for the problem, so maybe my modification is not the best method.
Hope this help you.

@jjqq2013 jjqq2013 changed the title Fix bug: cookie.expires not modified even if maxAge set and rolling:true Fix bug: cookie.expires not modified even if maxAge set Feb 21, 2016
@dougwilson dougwilson self-assigned this Mar 7, 2016
@gabeio gabeio mentioned this pull request Mar 28, 2016
Closed
@sboehler sboehler mentioned this pull request Mar 29, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@dougwilson dougwilson

Labels

needs tests pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jjqq2013 @dougwilson